Code Review: Chat - /files page - Drag-n-Drop Files

Summary

Adds POST /api/sandboxes/files endpoint to upload files from the drag-and-drop UI to the authenticated account's GitHub org submodule. Files are resolved through resolveSubmodulePath so they land in the correct sub-repo. Good test coverage across auth, validation, snapshot lookup, and upload result cases.

CLEAN Code Assessment

SRP ✅ commitFileToRepo handles GitHub, uploadSandboxFilesHandler handles orchestration — well separated.

OCP ✅ No modification to existing code paths.

DRY ✅ Reuses validateAuthContext, selectAccountSnapshots, resolveSubmodulePath.

YAGNI ✅ No over-engineering; handles exactly what was asked.

Issues Found

🔴 Blocking

1. Path traversal via file.name

const fullPath = `${folder.replace(/\/$/, "")}/${file.name}`;

file.name comes from the Content-Disposition header in the multipart body and is not sanitized. A crafted request could send file.name = "../../../.env", resulting in a path like .openclaw/workspace/orgs/myorg/../../../.env, which GitHub normalizes to .env at the repo root — allowing arbitrary file writes.

Fix: strip ../ sequences and leading slashes from file.name before building the path:

const safeName = file.name.replace(/(\.\.\/|\.\.\\/|^\/)/g, "").replace(/[\x00-\x1f]/g, "");
const fullPath = `${folder.replace(/\/$/, "")}/${safeName}`;

🟡 Suggestions

2. Hardcoded branch: "main" in commitFileToRepo

branch: "main",

This silently targets main regardless of the actual default branch. Repos using master or another default branch will fail. Consider either:

• Accepting branch as an optional parameter (defaulting to "main"), or
• Fetching the repo's default branch via GET /repos/{owner}/{repo} when not provided.

3. Unhandled selectAccountSnapshots rejection

The snapshots lookup isn't wrapped in try/catch. A database error will produce an unhandled 500. Given the pattern used in similar handlers, wrap in try/catch with a { status: 500 } response.

4. No file size limit

The handler accepts arbitrarily large files. Consider adding a limit (e.g., 10MB per file or per request) before calling file.arrayBuffer() to avoid memory exhaustion.

🔵 Nits

• Empty JSDoc blocks on createMockFormData and createMockRequest test helpers — remove them.

Security

🔴 Path traversal via file.name (see blocking issue above).
✅ Auth enforced via validateAuthContext.
✅ No hardcoded secrets.

Verdict: request-changes

Path traversal on file.name must be fixed before merging. The branch hardcoding and missing try/catch are worth addressing in the same PR.